Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dl_vlan validation to allow mask #135

Merged
merged 9 commits into from
Mar 8, 2023
Merged

Conversation

ajoaoff
Copy link

@ajoaoff ajoaoff commented Feb 16, 2023

Closes #132

  • Accept dl_vlan in the format vlan/mask in addition to vlan

Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajoaoff looks good to me.

Could you update the changelog? Also, could you provide either a migration MongoDB command in the changelog itself if it's short, or a migration script updating/mapping dl_vlan values as string? This change would break anything but it would leave different types in the collection that can become confusing, 2022.3.X will be used in production at AmLight, thanks.

@ajoaoff
Copy link
Author

ajoaoff commented Feb 17, 2023

@viniarck I'll provide the migration script, but anyway I don't think this PR should ship with 2022.3.x, it is a change for 2023.1. What do you think about creating a branch for the changes expected to ship with 2023.1?

@viniarck
Copy link
Member

viniarck commented Feb 20, 2023

@viniarck I'll provide the migration script, but anyway I don't think this PR should ship with 2022.3.x, it is a change for 2023.1. What do you think about creating a branch for the changes expected to ship with 2023.1?

@ajoaoff, yes, we won't ship to 2022.3.X, when I read my comment again, I summarized it too much, what I meant to write was, since AmLight will deploy 2022.3, and this will be shipped to master and eventually 2023.1, then ideally they can use this script before using/deploying 2023.1 to keep the collection with dl_vlan type the same.

Antonio, if you could also update the openapi.yml spec as well, that'd be great, we'll start reusing the openapi-core validate decorator that you've implemented to also validate some endpoints on flow_manager.

Regarding a new branch for 2023.1 although it's a great idea, at this time, I'd rather maintain master and then on upstream only keep tag and start creating old branches for patched versions since the tag by itself wouldn't suffice, sometimes we don't have a new version scope defined yet, and then contributions might also land on master. We can keep evolving this discussion if we can benefit from a simpler workflow.

@Alopalao
Copy link

Alopalao commented Feb 21, 2023

This PR also handles any and untagged cases I am working on. mef_eline issue #219

@ajoaoff ajoaoff requested a review from a team as a code owner February 23, 2023 00:35
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for also pushing this script @ajoaoff.

Antonio, before merging, let's also update the changelog, if you can also mention there about this script that'd be great. @italovalcy when AmLight upgrades to 2023.1 some months down the road you'll probably want to run this script.

@italovalcy
Copy link

@viniarck and @ajoaoff, since this is a major change that will impact other Napps, IMO we have to increase the API version and keep the old one compatible with the previous format—maybe providing an abstraction between the two versions (which will convert int to string).

Another strategy would be supporting both int and string.

WDYT?

@Alopalao
Copy link

Alopalao commented Mar 7, 2023

Good point. If we decide to unify the string format for dl_vlan, of_core will be affected since the vlan_id is returned as int (here). Combination of int and string is already supported, here vlan with mask it is returned as string.
Minor detail, it is easier/faster to differentiate strings from integers while writing code since they represent special cases.

@viniarck
Copy link
Member

viniarck commented Mar 7, 2023

Agreed @italovalcy @Alopalao, I tend to avoid mixing different type for the same field dl_vlan, but it's becoming evident that dl_vlan being Optional[Union[str, int]] would make things way simpler, and only use a str when setting a mask as of_core has been designed, not completely breaking compatibility with the API as well while extending it to support the mask. @ajoaoff could you adapt this part? That way we also wouldn't need the script as well. Thanks

Regarding bumping v3 though, let's leave it to when we address the other validations in general on issue #43 that we have on our radar (cookie_mask mandatory for deletions an so on).

@viniarck viniarck self-requested a review March 8, 2023 14:54
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajoaoff thanks for this PR and pushing the latest commits. I went ahead and finished reverting the scripts related changes and updated the changelog just so this PR can get merged right away unblocking also mef_eline uni any/untagged feature that @Alopalao is working on.

@viniarck viniarck merged commit bf6b75f into master Mar 8, 2023
@viniarck viniarck deleted the feat/support-vlan-range branch March 8, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support FlowMod matching vlan with mask
4 participants